-
-
Notifications
You must be signed in to change notification settings - Fork 387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add UserDetailTelegram support #1038
Conversation
41a931c
to
86040cd
Compare
Codecov Report
@@ Coverage Diff @@
## master #1038 +/- ##
==========================================
- Coverage 44.28% 44.18% -0.11%
==========================================
Files 126 126
Lines 2897 2904 +7
Branches 653 653
==========================================
Hits 1283 1283
- Misses 1602 1609 +7
Partials 12 12
Continue to review full report at Codecov.
|
@akellbl4 do you know why codecov is running on that MR which doesn't change anything in the frontend and is reporting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not checked much of the logic as it seems to me mostly an extension of the existing user email part of the managment. Just a couple of minor things
02e3c3b
to
429f9e4
Compare
// getNotificationEmails returns list of emails for notifications for provided comment. | ||
// Emails is not added to the returned list in case original message is from the same user as the notification receiver. | ||
func (s *Service) getNotificationEmails(req Request, notifyComment store.Comment) (result []string) { | ||
// getNotificationTargets returns list of notification targets (like email or telegram username) for users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment to reflect the new name, made results deduplicated to prevent the caller from doing it.
Pull Request Test Coverage Report for Build 933652465
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a comment to fix and good to go
429f9e4
to
411b8e4
Compare
For telegram notifications we'll need to get a telegram user ID from the user and store and retrieve it later, this PR adds support for that user details and improves a bunch of tests in affected modules.
All newly added code is tested.